Skip to content

Conversation

zhengli0060
Copy link
Contributor

This pull request fixes the logic for detecting uncovered potentially directed paths in FCI (rule 9).
Previously, certain cases (e.g., test_case_orient_rules) were not correctly identified, which could result in incorrect edge orientations.

Changes:

  • Added a new helper function traversePotentiallyDirected.
  • Added a new function existsUncoveredPdPath to determine whether an uncovered potentially directed path exists.
  • Updated the rule9 logic in fci.py to leverage these new functions for more accurate path checking.

Signed-off-by: Zheng Li <zhengli0060@gmail.com>
Copy link
Collaborator

@EvieQ01 EvieQ01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zhengli0060 for fixing rule9! The logic is correct and test went through pretty well.
One small problem: in the uniittest (TestDAG2PAG), it's not common to use ad-hoc manual calls for the testcase, and i suggest to delete the "if name == "main__"". Otherwise I think it's ready to merge.

Delete a test case.

Signed-off-by: ZhengLi <142206843+zhengli0060@users.noreply.github.com>
@zhengli0060
Copy link
Contributor Author

Thank you @zhengli0060 for fixing rule9! The logic is correct and test went through pretty well. One small problem: in the uniittest (TestDAG2PAG), it's not common to use ad-hoc manual calls for the testcase, and i suggest to delete the "if name == "main__"". Otherwise I think it's ready to merge.

Thanks for Evie’s review. I’ve removed the new test case as suggested.

@zhengli0060 zhengli0060 requested a review from EvieQ01 October 13, 2025 00:57
@EvieQ01
Copy link
Collaborator

EvieQ01 commented Oct 13, 2025

Thank you for checking again. Maybe there's a little understanding though: it's good to have the unittest case (def test_case_orient_rules), but to run the test, "if __name__ == '__main__': unittest.main()" would do the job rather than calling the tests separately. To keep the code clean and consisten with other test modules, I think L90-116 in the original pr is good to keep, but that L119-125 in the original pr should be removed.

Signed-off-by: ZhengLi <142206843+zhengli0060@users.noreply.github.com>
@zhengli0060
Copy link
Contributor Author

Thank you for checking again. Maybe there's a little understanding though: it's good to have the unittest case (def test_case_orient_rules), but to run the test, "if __name__ == '__main__': unittest.main()" would do the job rather than calling the tests separately. To keep the code clean and consisten with other test modules, I think L90-116 in the original pr is good to keep, but that L119-125 in the original pr should be removed.

Thank you for your patient explanation. I’ve updated it accordingly.

@EvieQ01
Copy link
Collaborator

EvieQ01 commented Oct 14, 2025

Perfect!

@EvieQ01 EvieQ01 closed this Oct 14, 2025
@EvieQ01 EvieQ01 reopened this Oct 14, 2025
@EvieQ01 EvieQ01 closed this Oct 14, 2025
@EvieQ01 EvieQ01 reopened this Oct 14, 2025
@kunwuz kunwuz merged commit d80bad1 into py-why:main Oct 14, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants